Improvements for sample app#121
Conversation
lisajulia
commented
Jul 17, 2026
lisajulia
commented
- extract supplier correctly
- turn off recommendations for status field
- extract supplier correctly - turn off recommendations for status field
SummaryThe following content is AI-generated and provides a summary of the pull request: Sample App: Supplier Extraction & Status Recommendation ImprovementsBug Fix / New Feature🛠️ Improved the bookshop sample app with two key enhancements: correct supplier matching from extracted invoice data and disabling UI recommendations for the status field. Changes
PR Bot InformationVersion:
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces four issues worth addressing: an invalid email in the CSV seed data, a full-table-scan supplier lookup that should push filtering to the database, redundant @title annotations that duplicate existing inline labels, and a @UI.RecommendationState: 0 annotation that doesn't actually disable recommendations as the PR description claims.
PR Bot Information
Version: 1.28.0
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
dd0f0370-81ad-11f1-9a3d-6a352c785d60 - File Content Strategy: Full file content
- Event Trigger:
pull_request.opened
| c2f3d4e5-6a7b-8c9d-0e1f-2a3b4c5d6e7f;O'Reilly Media;US;invoices@oreilly.com | ||
| d3a4b5c6-7d8e-9f0a-1b2c-3d4e5f6a7b8c;Elsevier;NL;ap@elsevier.com | ||
| e4b5c6d7-8e9f-0a1b-2c3d-4e5f6a7b8c9d;Pearson Education;GB;finance@pearson.com | ||
| e4b5c6d7-8e9f-0a1b-2c3d-4e5f6a7b8c9f;Studio Salford;US;@pearson.com |
There was a problem hiding this comment.
Bug: Invalid email address for new supplier entry
The email value @pearson.com is missing the local part before the @ sign, making it an invalid email address. This will likely cause validation errors or be rejected by email-handling logic.
Consider providing a valid email address such as ap@studiosalford.com.
| e4b5c6d7-8e9f-0a1b-2c3d-4e5f6a7b8c9f;Studio Salford;US;@pearson.com | |
| e4b5c6d7-8e9f-0a1b-2c3d-4e5f6a7b8c9f;Studio Salford;US;ap@studiosalford.com |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| private Optional<Suppliers> findSupplierByName(String name) { | ||
| String normalized = name.toLowerCase(Locale.ROOT); | ||
| return db.run(Select.from(Suppliers_.class)) |
There was a problem hiding this comment.
Performance: findSupplierByName fetches the entire Suppliers table into memory to do a case-insensitive name match in Java.
For even a modest number of suppliers this is wasteful — every extraction event loads all supplier rows. The filter should be pushed down to the database using a where clause with a lower-case comparison, or at minimum a where(s -> s.name().eq(name)) combined with post-filter. Consider:
return db.run(Select.from(Suppliers_.class)
.where(s -> s.name().eq(name)))
.listOf(Suppliers.class)
.stream()
.filter(s -> s.getName() != null && normalized.equals(s.getName().toLowerCase(Locale.ROOT)))
.findFirst();This at least avoids a full-table scan for exact matches and falls back to the in-memory case-insensitive check only when needed.
| return db.run(Select.from(Suppliers_.class)) | |
| return db.run(Select.from(Suppliers_.class) | |
| .where(s -> s.name().eq(name))) | |
| .listOf(Suppliers.class) | |
| .stream() | |
| .filter(s -> s.getName() != null && normalized.equals(s.getName().toLowerCase(Locale.ROOT))) | |
| .findFirst(); |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| annotate service.SupplierInvoices with { | ||
| invoiceNumber @title: '{i18n>InvoiceNumber}'; | ||
| supplier @title: '{i18n>Supplier}'; | ||
| status @title: '{i18n>Status}'; | ||
| } |
There was a problem hiding this comment.
Best Practices: The new @title annotations for invoiceNumber, supplier, and status duplicate labels already defined inline on every LineItem and FieldGroup entry in the same file.
@title on a property acts as a default label and is useful when no explicit Label is provided. Since all existing LineItem and FieldGroup data fields already carry explicit Label keys that match these titles, this block adds no functional value and creates a maintenance risk (two places to update labels). Consider removing the block or replacing the per-field Label: properties with this single source of truth instead.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
